-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: messages content overlap when bottom sheet is shown #42143
base: main
Are you sure you want to change the base?
fix: messages content overlap when bottom sheet is shown #42143
Conversation
@Santhosh-Sellavel Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
54ae3d2
to
00d252f
Compare
Tagging @rojiphil here. |
Just a quick update - after enabling new architecture I see very low FPS for animation: Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-05-14.at.18.11.00.mp4Currently trying to understand why it works so slow and what needs to be changed to speed it up 👀 Also found a crash and fixed it in kirillzyusko/react-native-keyboard-controller#444 Keep working on this PR 💪 |
d3eed50
to
55eef53
Compare
55eef53
to
acfbdcb
Compare
I continue to work on this problem. Still investigating why we observe performance drop - the problem is that even if we don't change layout of list/footer etc. and run a simple animation of red square (via Screen.Recording.2024-06-06.at.13.02.07.movI think this investigation will take more time, than I initially expected. |
@kirillzyusko I'm going to mark this as WIP, feel free to remove the prefix when you feel it's ready for further reviews |
…/10632-when-long-press-on-message-add-space-for-image-squashed-commits
Reviewer Checklist
Screenshots/VideosiOS: Native42143-ios-native-002.mp4iOS: mWeb Safari42143-mweb-safari-001.mp4Android: Native42143-android-native-001.mp4Android: mWeb Chrome42143-mweb-chrome-001.mp4MacOS: Chrome / Safari42143-web-safari-001.mp4MacOS: Desktop42143-desktop-001.mp4 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @kirillzyusko for the changes and the only pending review comments to be addressed are here
@mountiny @luacmartins Changes LGTM and tests well too.
Although not related to our PR here but there is a console warning that comes up for which I have raised the issue in slack here.
All yours now. Thanks.
…ce-for-image-squashed-commits
Both the reassure tests and lint failures seem related and should be addressed |
Tests started to fail after 6140d8f I'll investigate why on Monday - now will focus on fixing all new issues coming from edge-to-edge mode |
…ce-for-image-squashed-commits
@kirillzyusko Merge main, so we can retest with nav changes. |
@kirillzyusko let's merge main to get nav changes and also address the failing eslint check |
@luacmartins sure, was planning to return back to this task tomorrow! Hopefully I can fix all failing tests so it should be ready for another round of review tomorrow! |
…ce-for-image-squashed-commits
Thank you! |
Okay, eslint complains only about useOnyx/withOnyx - I think this PR can be tested/reviewed again. All other checks are green ✅ |
@shubham1206agra @rojiphil this PR is ready for testing again 😊 |
🚧 @mountiny has triggered a test build. You can view the workflow run here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@luacmartins @mountiny The post review changes LGTM and tests well too.
iOS: Native
42143-ios-native-005.mp4
iOS: mWeb Safari
42123-mweb-safari-005.mp4
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Screen.Recording.2024-12-12.at.7.43.41.PM.mov@kirillzyusko BUG: Extra padding gets introduced to edit composer |
Screen.Recording.2024-12-12.at.7.55.32.PM.movBUG: Weird stutter happening on going to search |
@kirillzyusko Conflicts in PR |
Details
A successor of #16356
The first step of react-native-keyboard-controller migration plan
Important
This PR relies on
react-native-keyboard-controller
(useKeyboardHandler
) hook andKeyboardAvoidingView
component).How it works 🤔
This PR is iOS only for now. Other platforms are not affected. Also, in this explanation, we are talking about iOS only since on Android, iOS web and Android Web keyboard handling works differently.
Note
We're talking about default platform behavior. If we start to use
react-native-keyboard-controller
on Android - it'll disable all default keyboard handling by OS and will delegate the keyboard handling to us - so we will have identical behavior across both iOS/Android platforms.But it may introduce additional issues (because we need to enter edge-to-edge mode - that would be good to do on entire app level, but it may bring some issues with
StatusBar
paddings, so everything has to be verified very carefully). That's why I think it would be better to handle in separate PR.When we long press on a message, we want to be able to see it. It's easy when we have the keyboard closed. But gets tricky when the keyboard is open.
But first of all let's focus on the case when the keyboard is closed. User long press on the message, we render the bottom sheet (action sheet) with some options. If the message is higher than the height of bottom sheet - the message is visible. When the message is the last one (first from the bottom), it can be fully or partially covered by the bottom sheet.
Press to see videos
300481023-9dc8be61-799d-470b-997f-ee5c5995fe19.mp4
300481277-0d0570cd-a85f-4cf5-8a91-6a3d5ad72c58.mp4
Also everything is tricky when we have keyboard open 😓
Press to see videos with keyboard
300482884-cbe3b610-fa85-4071-a3dd-bbc1a9d56e05.mp4
300483006-2e5ba833-c0fa-4fad-b383-b3854e5fa112.mp4
To achieve this goal we had to:
FlatList
ofReportListView
(it uses reanimated to move the view by the offset)KeyboardAvoiding
so we're in sync with keyboard-avoiding style updatesActionSheetAwareScrollViewContext
to pass the transition function, so we can transition from one state to another by action triggered from different parts of the applicationBelow you can find more use cases that involves keyboard and other UI elements interactions:
Interactions with other UI elements
300483536-04991f52-a41b-4d3e-abaf-12712b94f1aa.mp4
300483723-91b076a9-4c7a-4fbb-bc50-1e8194763e9d.mp4
Note
Call popover, bottom sheet and emoji picker are implemented as Modal windows and on iOS keyboard instantly gets hidden when modal window is shown. But "Attachment" popup is implemented not as modal, so for this type of transition we are using
progress
-based animations. When we press on plus -> popup is already shown under keyboard and we dismiss the keyboard. Overall the mechanism is similar to what we've used in the past, except the moment where we substitutetranslationY
value - in case when keyboard disappear/appears we interpolate it byprogress
variable to achieve smooth transitions.Technical details ⚒️
So in order to calculate the offset for the message, we:
ref.current.measureInWindow
, so we can haveheight
andy
coordinate of the imageonLayout
callbackDimensions
safeArea.top
sincey
coordinate doesn't get into account safe areaelementOffset = (y + safeArea.top + itemHeight) - (windowHeight - popoverHeight)
In order to animate item when they keyboard was open, we need to also know its height which is provided by useAnimatedKeyboard hook from react-native-reanimated, but we should always subtract safeArea.bottom from it since it's not part of the calculation for the offset, but only keyboardHeight.
Overall, the keyboard is the tricky part. On iOS, the keyboard doesn't resize the viewport. So for the content to not be covered by the keyboard,
KeyboardAvoidingView
is used. In React Native,KeyboardAvoidingView
is implemented as a view that changes its height or resizes the children's view by the keyboard height. It's done using RN's Layout Animations. This results in the animation for the keyboard being applied using the native event emitter callback which uses the bridge. So open/close events of the keyboard will always be delayed that resulting in the delayed keyboard avoiding animation and, which is more important, scheduling layout animations.In order to be consistent 100% of the time with animation and timings, we had to:
KeyboardAvoidingView
fromreact-native-keyboard-controller
for iOS. It reacts on keyboard height/state change on UI thread in the same frame, so we can schedule an update for styles in the next frameuseDerivedValue
for all the logic, since introducinguseAnimatedReaction
or having multiple shared values will delay updates for 1 frame, but we want to be 100% in sync with keyboard updatespaddingTop: translateY
(becauseScrollView
is inverted).Another important aspect of the implementation is the usage of the state machine for the animation. State machine allows us to specify the chain of events and how to handle them, and more importantly what actions to ignore, for example:
KEYBOARD_OPEN
actionEMOJI_PICKER_OPEN
actionEMOJI_PICKER_CLOSE
action which again does nothingKEYBOARD_OPEN
action. idle can react to this action by transitioning into keyboardOpen stateKEYBOARD_CLOSE
,EMOJI_PICKER_OPEN
actionsEMOJI_PICKER_OPEN
action which transitions our state intoemojiPickerOpen
state. now we react only toEMOJI_PICKER_CLOSE
actionKEYBOARD_CLOSE
action. but we ignore it since ouremojiPickerOpen
state can only handleEMOJI_PICKER_CLOSE
action. so we write the logic for handling 9. hiding the keyboard but maintaining the offset based on the keyboard state shared valueEMOJI_PICKER_CLOSE
action which transitions us back intokeyboardOpen
state.Fixed Issues
$ #10632
PROPOSAL: N/A
Tests
These steps were performed on all the platforms. The new behavior is iOS only for now.
Before each next we make sure the keyboard is open:
Offline tests
Should not affect the offline state.
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
telegram-cloud-document-2-5460627828225625066.mp4
Android: mWeb Chrome
telegram-cloud-document-2-5460627828225625138.mp4
iOS: Native
1104ff6c-6eba-4295-a905-6caaaf3edf28.mp4
iOS: mWeb Safari
6eff007f-3c61-4f19-a60e-43f46d2f0e61.mp4
MacOS: Chrome / Safari
e5e28bd8-529f-4da3-8ae7-bff03bd14499.mp4
MacOS: Desktop
2c813bc9-6988-4fbc-8f36-3282c9cecd82.mp4